-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: sync extension bridge settings with cloud #7535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Use CloudService.getUserSettings() for remoteControlEnabled instead of global state - Update CloudService.updateUserSettings when toggling remote control - Add BridgeOrchestrator.connectOrDisconnect handling in settings update handler - Remove dependency on contentProxy/globalSettings for remote control state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| extensionBridgeEnabled: message.bool ?? false, | ||
| }) | ||
| } catch (error) { | ||
| provider.log(`Failed to update cloud settings for remote control: ${error}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the cloud settings update fails, we're only logging the error. Should we also show a user-facing notification so they know their setting change didn't persist?
| provider.log(`Failed to update cloud settings for remote control: ${error}`) | |
| } catch (error) { | |
| provider.log(`Failed to update cloud settings for remote control: ${error}`) | |
| vscode.window.showErrorMessage('Failed to update remote control setting. Please try again.') | |
| // Don't fall back to local storage - cloud settings are the source of truth | |
| } |
| sessionId: vscode.env.sessionId, | ||
| }) | ||
| } catch (error) { | ||
| cloudLogger( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that we're not notifying the user when BridgeOrchestrator update fails? The error is logged but users might want to know if their remote control connection has issues.
| settingsUpdatedHandler = postStateListener | ||
|
|
||
| // Enhanced settings updated handler that also updates BridgeOrchestrator | ||
| settingsUpdatedHandler = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding JSDoc comments to document this enhanced settings handler's functionality, especially since it now handles BridgeOrchestrator updates in addition to posting state.
This PR implements cloud-based settings synchronization for the extension bridge feature, replacing local storage with CloudService as requested.
Changes
CloudService.getUserSettings()?.settings?.extensionBridgeEnabledinstead of global state forremoteControlEnabledCloudService.updateUserSettingswhen toggling remote control in the webviewBridgeOrchestrator.connectOrDisconnecthandling in the settings update handlerextensionBridgeEnabledinClineProvider.getState()responseCore Goals Achieved
✅ Don't use contentProxy / globalSettings remoteControlEnabled, use CloudService.instance.getUserSettings()?.settings?.extensionBridgeEnabled
✅ Use CloudService.instance.updateUserSettings to update extensionBridgeEnabled as part of handling the "remoteControlEnabled" webview message
✅ Include CloudService.instance.getUserSettings()?.settings?.extensionBridgeEnabled in the ClineProvider.getState() response under the remoteControlEnabled property
✅ Update BridgeOrchestrator using connectOrDisconnect when cloud user settings change
Testing
Reference
Based on commit dace718 (jr/cloud-control-sync) but using the new BridgeOrchestrator class as requested.
Important
This PR syncs the extension bridge settings with the cloud, replacing local storage with
CloudServicefor managingextensionBridgeEnabledand updating related components accordingly.remoteControlEnabledwithCloudService.getUserSettings()?.settings?.extensionBridgeEnabledinClineProvider.tsandwebviewMessageHandler.ts.CloudService.updateUserSettingswhen toggling remote control in the webview.BridgeOrchestrator.connectOrDisconnectinextension.tsandClineProvider.tsto handle cloud settings changes.extensionBridgeEnabledinClineProvider.getState()response.settingsUpdatedHandlerinextension.tsto reflect cloud settings changes.ClineProvider.tsandwebviewMessageHandler.ts.This description was created by
for c512344. You can customize this summary. It will automatically update as commits are pushed.